New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Association display #1015

Merged

jrchudy merged 19 commits into master from association-display

Oct 3, 2024

Member

jrchudy commented Aug 27, 2024 •

edited

Loading

This PR adds a BulkCreateForeignKeyObject object to ermrestJS that is attached to reference once the compute function is called to initialize the object.

I left a PR comment about prefillObject maybe being here instead of chaise and another one about composite keys.

Test cases added in this PR:

table has 2 FKs + 3 columns that are NOT system columns
- both FKs together are part of the same key
table has 2 FKs that are NOT system columns
- the FKs are no part of key information
table has 3 FKs that are NOT system columns
table has a composite foreign key from the association table to the main table
- ensures this feature isn't triggered for composite FKs
pure and binary association table

jrchudy added 5 commits

June 27, 2024 11:54


          isAssocation API added

4b5536b


          Merge branch 'master' into association-display

9f9ac9f


          remove old temprary changes and add a new object to reference.js call…

c30f722

…ed Prefill


          Merge branch 'master' into association-display

19f3295


          remove arrow synatx and other bug fixes

6f596fd

jrchudy self-assigned this

jrchudy requested a review from RFSH

August 27, 2024 00:02

jrchudy commented

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

jrchudy commented

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

jrchudy commented

View reviewed changes

js/reference.js Show resolved Hide resolved

jrchudy commented

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

jrchudy commented

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

RFSH requested changes

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

js/reference.js Outdated Show resolved Hide resolved

js/reference.js Outdated Show resolved Hide resolved

jrchudy added 5 commits

September 3, 2024 12:16


          Update changes based on comments. Tests pending

f479a18


          fix bugs in ermrestJS after refactoring

81a9cdc


          change constructor name, add test cases

f03b2f2


          Merge branch 'master' into association-display

15bc467


          update comment

50f593c

jrchudy commented

View reviewed changes

js/reference.js Show resolved Hide resolved

jrchudy requested a review from RFSH

September 13, 2024 23:11

Member Author

jrchudy commented Sep 19, 2024

@RFSH This is ready for review now with the changes to chaise that are ready. We should discuss about the 2 comments I left in this PR about:

how do we make sure the foreign key is not part of a composite foreign key
should the prefillObject be a property of reference.bulkCreateForeignKey or stay as a state variable in chaise


          make sure 2 foreign keys are not composite and add a test case for that

97d5967

RFSH reviewed

View reviewed changes

js/reference.js Outdated Show resolved Hide resolved

RFSH reviewed

View reviewed changes

js/reference.js Show resolved Hide resolved

RFSH reviewed

View reviewed changes

js/reference.js Show resolved Hide resolved

jrchudy added 2 commits

September 25, 2024 15:20


          return null instead of an error, add comments

6176ef7


          add a new 'context' for reference for selecting foreign key values in…

78bd7b2

… bulk in recordedit

jrchudy commented

View reviewed changes

js/reference.js Show resolved Hide resolved

jrchudy commented

View reviewed changes

js/utils/constants.js Show resolved Hide resolved

jrchudy requested a review from RFSH

September 26, 2024 20:22

jrchudy added 2 commits

September 30, 2024 12:41


          function for generating the unique id for a row when we have the fk p…

945014d

…seudo column and the row data


          add test for pseudocolumn generateUniqueId function

6e9f2bd

RFSH reviewed

View reviewed changes

js/column.js Outdated

+              /**
+               * Given the available tuple data, generate the uniqueId for the selected row from the table this pseudo column points to
+               *
+               * @param {*} linkedData

Member

RFSH Sep 30, 2024

Can you please add a small description to this param that clarifies what this is. It's the key-value pair for the column values of the table that this pseudo column points to. I'm saying this because linkedData (Tuple.linkedData and what we're passing to .filteredRef) is not just a key-value pair of one table. It has the data for all the outbound fks.

RFSH approved these changes

View reviewed changes

Member

RFSH left a comment

I think everything looks good and works as expected. I like the name that you used for the new context. Can you please add the new context to the "Context names" section of annotation doc.

You should also do make all, so it generates the new api.md and then push it.

jrchudy added 4 commits

October 1, 2024 10:29


          added more info to comments

c15b13b


          Merge branch 'master' into association-display

5b545c3


          semicolons and api.md updates


          update annotation.md

0ba99f2

jrchudy merged commit 7c354b9 into master

1 check passed

jrchudy deleted the association-display branch

October 3, 2024 16:57

jrchudy mentioned this pull request

Bulk create foreign key property added to annotations #1018

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet